Block editor: rewrite selection clearer #27468
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
force-pushed
the
remove/focusable-wrapper-div
branch
from
6b3a907 to
4538409
Compare
|
Size Change: +29 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
ellatrix
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
ellatrix
force-pushed
the
remove/focusable-wrapper-div
branch
from
bf85a9c to
6161bfa
Compare
ellatrix
changed the title
ellatrix
force-pushed
the
remove/focusable-wrapper-div
branch
from
4b46a86 to
c34a199
Compare
ellatrix
commented
Dec 6, 2020
| function onFocus() { | ||
| function onMouseDown( event ) { | ||
| // Only handle clicks on the canvas, not the content. | ||
| if ( event.target.closest( '.wp-block' ) ) { |
There was a problem hiding this comment.
This condition could be removed eventually when the content is directly rendered into the wrapper the mousedown event is listened for. Currently there are a few wrappers in between the selection clearer and the block for which we also need to detect clicks.
There was a problem hiding this comment.
The changes in this PR break the selection clearer in the widgets screen (you can't click any button on the toolbar). .wp-block must not be enough.
Ii also wonder why you're using mousedown, focusing outside (without clicking) should also have the same behavior right?
There was a problem hiding this comment.
I'm going to restore the previous behavior for now to perform a patch release.
|
I think this kind of impactful PRs could use more reviews. |
youknowriad
reviewed
Dec 25, 2020
| @@ -64,7 +64,6 @@ export default function VisualEditor() { | |||
| <div | |||
| ref={ ref } | |||
| className="editor-styles-wrapper" | |||
| tabIndex="-1" | |||
There was a problem hiding this comment.
Do we know why this was here and why this was removed?
youknowriad
added a commit
that referenced
this pull request
Dec 25, 2020
This reverts commit 6c9efd4.
github-actions
bot
added
the
[Status] In Progress
youknowriad
added a commit
that referenced
this pull request
Dec 25, 2020
youknowriad
added a commit
that referenced
this pull request
Dec 25, 2020
ellatrix
added a commit
that referenced
this pull request
Jan 8, 2021
ellatrix
added a commit
that referenced
this pull request
Jan 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently the selection clearer works by catching focus on a wrapper. Because of this focusable element, it's trickier to remove the wrapper and share it with the click redirect wrapper. Additionally, it also tricky to remove in the context of an iframe where the
bodyelement cannot be truly focusable (because of how the browser handler focus loss, returning focus to thebodyelement in that case, without firing any focus events).The selection clearer can easily be rewritten to look for clicks instead of focus. There's already a precedent of doing so in the click redirect, where we redirect focus to the last block if the user clicks at the bottom of the editor. The clearer is basically the same thing, detecting clicks around the content to clear the block selection.
How has this been tested?
Screenshots
Types of changes
Checklist: